RDKB-62636 : Stop running DHCP client before starting a new one via DHCP Manager#191
RDKB-62636 : Stop running DHCP client before starting a new one via DHCP Manager#191S-Parthiban-Selvaraj wants to merge 1 commit intomainfrom
Conversation
Add WanManager_StopDhcpClientIfRunning() under FEATURE_RDKB_DHCP_MANAGER
that queries Device.DHCPv4.Client.{i}.Status / Device.DHCPv6.Client.{i}.Status
from the DHCP Manager. If the status is Enabled, the running client is stopped
(without release) before the new start proceeds.
Called from WanManager_StartDhcpv4Client and WanManager_StartDhcpv6Client
to avoid duplicate DHCP client instances.
There was a problem hiding this comment.
Pull request overview
Adds a DHCP Manager–based guard to prevent starting duplicate DHCP client instances by stopping any already-running DHCPv4/DHCPv6 client before initiating a new start.
Changes:
- Introduces
WanManager_StopDhcpClientIfRunning()underFEATURE_RDKB_DHCP_MANAGER. - Queries DHCP client status via DHCP Manager DML and stops an already “Enabled” client (without release).
- Calls the new helper from both DHCPv4 and DHCPv6 start paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| source/WanManager/wanmgr_net_utils.h | Declares the new DHCP Manager helper API and documents intended behavior/return values. |
| source/WanManager/wanmgr_net_utils.c | Implements the helper and invokes it before starting DHCPv4/DHCPv6 clients to avoid duplicates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ANSC_STATUS WanManager_StopDhcpClientIfRunning(DML_VIRTUAL_IFACE* pVirtIf, BOOL isIPv4) | ||
| { | ||
| if (pVirtIf == NULL) | ||
| { | ||
| CcspTraceError(("%s %d: Invalid args\n", __FUNCTION__, __LINE__)); | ||
| return ANSC_STATUS_FAILURE; | ||
| } | ||
|
|
||
| char dmlStatus[256] = {0}; | ||
| char statusValue[64] = {0}; | ||
| const char *dhcpIface = isIPv4 ? pVirtIf->IP.DHCPv4Iface : pVirtIf->IP.DHCPv6Iface; | ||
|
|
||
| /* Build the Status DML path, e.g. Device.DHCPv4.Client.1.Status */ | ||
| snprintf(dmlStatus, sizeof(dmlStatus), "%s.Status", dhcpIface); | ||
|
|
||
| if (ANSC_STATUS_SUCCESS == WanMgr_RdkBus_GetParamValues(DHCPMGR_COMPONENT_NAME, DHCPMGR_DBUS_PATH, dmlStatus, statusValue)) | ||
| { | ||
| CcspTraceInfo(("%s %d: %s current status = %s\n", __FUNCTION__, __LINE__, dmlStatus, statusValue)); | ||
| if (strncasecmp(statusValue, "Enabled", 7) == 0) | ||
| { | ||
| CcspTraceInfo(("%s %d: %s client is already running on %s, stopping it first\n", | ||
| __FUNCTION__, __LINE__, isIPv4 ? "DHCPv4" : "DHCPv6", pVirtIf->Name)); | ||
| if (isIPv4) | ||
| { | ||
| WanManager_StopDhcpv4Client(pVirtIf, STOP_DHCP_WITHOUT_RELEASE); | ||
| } | ||
| else | ||
| { | ||
| WanManager_StopDhcpv6Client(pVirtIf, STOP_DHCP_WITHOUT_RELEASE); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| CcspTraceWarning(("%s %d: Failed to get status for %s, proceeding with start\n", __FUNCTION__, __LINE__, dmlStatus)); | ||
| } | ||
|
|
||
| return ANSC_STATUS_SUCCESS; | ||
| } |
There was a problem hiding this comment.
This helper returns ANSC_STATUS_SUCCESS in all non-NULL cases and ignores the return values from WanManager_StopDhcpv4Client() / WanManager_StopDhcpv6Client(). That makes it impossible for callers to detect failures and also contradicts the function’s stated contract (failure on error). Consider (a) capturing and propagating the stop call’s return value when a stop is attempted, and (b) deciding whether a DHCP Manager status query failure should return ANSC_STATUS_FAILURE or updating the contract to explicitly describe the “best effort, always proceed” behavior.
| * @return ANSC_STATUS_SUCCESS if the client was not running or was stopped | ||
| * successfully, ANSC_STATUS_FAILURE on error. |
There was a problem hiding this comment.
The header comment states the function returns ANSC_STATUS_FAILURE on error, but the implementation logs a warning when the status query fails and still returns success (and also doesn’t surface stop failures). Either align the implementation to return failure for those error conditions, or update this docstring to reflect the intended best-effort semantics (e.g., always returns success unless arguments are invalid).
| * @return ANSC_STATUS_SUCCESS if the client was not running or was stopped | |
| * successfully, ANSC_STATUS_FAILURE on error. | |
| * @return ANSC_STATUS_SUCCESS on best-effort completion, including when no | |
| * client was running or when stopping the client fails internally. | |
| * ANSC_STATUS_FAILURE is only returned if the input arguments are | |
| * invalid. |
| if (ANSC_STATUS_SUCCESS == WanMgr_RdkBus_GetParamValues(DHCPMGR_COMPONENT_NAME, DHCPMGR_DBUS_PATH, dmlStatus, statusValue)) | ||
| { | ||
| CcspTraceInfo(("%s %d: %s current status = %s\n", __FUNCTION__, __LINE__, dmlStatus, statusValue)); | ||
| if (strncasecmp(statusValue, "Enabled", 7) == 0) |
There was a problem hiding this comment.
Using strncasecmp(..., \"Enabled\", 7) will treat values like \"EnabledXYZ\" as enabled. If DHCP Manager’s Status is expected to be an exact token (e.g., Enabled / Disabled), prefer an exact comparison (case-sensitive or case-insensitive as appropriate) or validate a full-token match (e.g., ensure the next character is string terminator/whitespace) to avoid false positives.
Add WanManager_StopDhcpClientIfRunning() under FEATURE_RDKB_DHCP_MANAGER that queries Device.DHCPv4.Client.{i}.Status / Device.DHCPv6.Client.{i}.Status from the DHCP Manager. If the status is Enabled, the running client is stopped (without release) before the new start proceeds.
Called from WanManager_StartDhcpv4Client and WanManager_StartDhcpv6Client to avoid duplicate DHCP client instances.